Skip to content

[Payment Method Improvements] Cash Payment Improvements #11414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 36 commits into from
May 15, 2024
Merged

[Payment Method Improvements] Cash Payment Improvements #11414

merged 36 commits into from
May 15, 2024

Conversation

backwardstruck
Copy link
Contributor

@backwardstruck backwardstruck commented Apr 29, 2024 •
edited
Loading

Closes: #11413

Description

This PR introduces a new "Change Due" calculator screen for merchants processing cash payments, enhancing the existing cash handling functionalities in our mobile app.

The new screen will only be visible when the 'OTHER_PAYMENT_METHODS' feature flag is enabled, ensuring that this functionality can be tested incrementally and does not impact users negatively during the roll-out phase.

Testing instructions

  1. Turn on the 'OTHER_PAYMENT_METHODS' feature flag.
  2. Navigate to the payment capture screen and select the cash payment option.
  3. Verify that the "Change Due" calculator screen appears

The following will be in a subsequent PR for easier review:

  • The "Cash Received" field displays the total amount due by default.
  • The UI corresponds to the new design specifications and not the current iOS design.
  • Edit the amount in the "Cash Received" field and ensure the displayed change due updates accordingly.
  • Tap "Mark order as complete", and confirm that it functions similarly to the "Mark as paid" button in the current dialog.
  • The new calculator will allow merchants to easily calculate the change owed to a customer when capturing a cash payment.
  • Complete unit test(s)

Images/gif

Images or gifs demonstrating the new screen and comparisons to the previous version will be attached here.

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@backwardstruck backwardstruck added type: enhancement A request for an enhancement. status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Apr 29, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Apr 29, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commit3a5645c
Direct Downloadwoocommerce-prototype-build-pr11414-3a5645c.apk

kidinov
kidinov reviewed May 2, 2024
View reviewed changes
kidinov
kidinov reviewed May 2, 2024
View reviewed changes
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually everything looks correct to me 👍 I just left a few minor suggestions

@backwardstruck backwardstruck marked this pull request as ready for review May 13, 2024 19:36
backwardstruck
backwardstruck commented May 13, 2024
View reviewed changes
...erce/src/test/kotlin/com/woocommerce/android/ui/payments/ChangeDueCalculatorViewModelTest.kt Outdated
import org.mockito.kotlin.whenever

@ExperimentalCoroutinesApi
class ChangeDueCalculatorViewModelTest : BaseUnitTest() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this unit test but I'm not sure there's much to test yet. Can I leave these TODO's for the next PR?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Fine to fix in the next PR. Think of it as test-driven development. You might even ticket that out: "Add implementation so order details load successfully emits success state test passes"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it as a task on this ticket. I can easily convert to GH issue if it ends up being a separate PR from the other tasks.

@backwardstruck backwardstruck added unit-tests-exemption and removed status: do not merge Dependent on another PR, ready for review but not ready for merge. labels May 13, 2024
@kidinov kidinov self-requested a review May 14, 2024 09:30
kidinov
kidinov reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @backwardstruck

Thanks @kidinov This should now be updated and ready for review. Not sure why I can't get it to pass CI 😢

There are tests that are failing. Click on "details" next to the failed job and look up what tests should be fixed

Overal looks good to me. I left a few comments. Please take a look

@kidinov kidinov self-assigned this May 14, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 36.84211% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 40.81%. Comparing base (00bd4c5) to head (81fe1fc).

Files Patch % Lines
...ts/methodselection/ChangeDueCalculatorViewModel.kt 0.00% 18 Missing ⚠️
...ts/methodselection/SelectPaymentMethodViewModel.kt 68.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##              trunk   #11414      +/-   ##
============================================
- Coverage     40.83%   40.81%   -0.02%     
- Complexity     5180     5181       +1     
============================================
  Files          1070     1071       +1     
  Lines         62310    62335      +25     
  Branches       8499     8500       +1     
============================================
+ Hits          25444    25445       +1     
- Misses        34578    34602      +24     
  Partials       2288     2288

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@backwardstruck
Copy link
Contributor Author

@kidinov it looks like CI is passing now with those changes. Let me know if you have additional suggestions.

@kidinov kidinov self-requested a review May 15, 2024 08:33
kidinov
kidinov approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I left one NP comment regarding visibility of a method. Feel free to ignore it

@backwardstruck backwardstruck enabled auto-merge May 15, 2024 14:10
@backwardstruck backwardstruck merged commit c0eb19a into trunk May 15, 2024
14 checks passed
@backwardstruck backwardstruck deleted the 10941-cash-payment-improvements branch May 15, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soundsokay soundsokay soundsokay left review comments

@kidinov kidinov kidinov approved these changes

Assignees

@kidinov kidinov

Labels
feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: enhancement A request for an enhancement. unit-tests-exemption
Projects
None yet
Milestone
18.7
Development

Successfully merging this pull request may close these issues.

Integration of New Calculator Screen with Feature Flag Implementation
6 participants
@backwardstruck @wpmobilebot @dangermattic @codecov-commenter @soundsokay @kidinov